-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normalise daoSnapshotENS
to lower case
#2247
Conversation
✅ Deploy Preview for decent-interface-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't go through the whole PR yet, but saw enough to stop.
Please see my comments! Thank you sir
src/components/DaoCreator/formComponents/EstablishEssentials.tsx
Outdated
Show resolved
Hide resolved
src/components/pages/DaoSettings/components/MetadataContainer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same theme of the ongoing discussion on this PR, I'd much rather the "lower-casing" happens only when it is input from the user via the Decent app.
Thus, as long as the ENS string came from Decent, it should be (and in fact IS) guaranteed to be lower case, so we should not need to lower case them anywhere else in-app. If it happens to be mixed case, then it is most certainly not originally from Decent (these guarantees, of course, are after this PR goes live), and I don't think we have any business insisting that it MUST be lowercased for whoever decided (or whatever) that they'd like their name non-standardised.
2887094
to
fe42cf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DarksightKellar i would expect that these two inputs are the two places that this PR should touch. What change are you requesting? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mudrila maybe this is why @DarksightKellar requested changes... does this PR do anything anymore? It doesn't seem like any of the changes here do the thing I was looking for: lowercasing text typed into those two input fields Kelvin screenshotted
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Closes #2234